-
Notifications
You must be signed in to change notification settings - Fork 32
[FSSDK-11522] feat: update decision service to apply ho #576
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
… use ExperimentCore instead of Experiment. Fix ActivateNotification, ExperimentUtils, ActivateNotificationListener, NotificationCenterTest to reflect these changes.
- Add logic to process holdout rules for feature flags - Implement a method to determine variation for a holdout rule - Create decision responses based on a holdout decision and its reasons - Update logger messages for holdout rule evaluation
…ctConfigV4 - Change visibility of FEATURE_FLAG_BOOLEAN_FEATURE to public - Change visibility of Variation VARIATION_HOLDOUT_VARIATION_OFF to public - Update entityIds in HOLDOUT_BASIC_HOLDOUT and holdouts-project-config.json to use "$opt_dummy_variation_id" instead of hardcoded values
- Create test for checking if a user is in a variation for holdout feature when included in flags - Implement test to verify exclusion of user from variation for holdout when specified as excluded from flags - Add assertions to validate decision source, experiment, and variation for holdout feature
- Added public constant HOLDOUT_TYPEDAUDIENCE_HOLDOUT to class ValidProjectConfigV4 - Updated feature flag constants to public static in class ValidProjectConfigV4 - Updated holdout constants to public static in class ValidProjectConfigV4 - Created unit test userMeetsHoldoutAudienceConditions in DecisionServiceTest
…ith_holdout' to accurately reflect the functionality being tested
… metadata in OptimizelyUserContextTest
- Implement createOptimizelyWithHoldouts method to create Optimizely instance with holdouts - Add test for decisionNotification with holdout scenario - Update decide_for_keys_with_holdout test for holdout variations - Implement decide_all_with_holdout test for multiple flag keys with holdouts
- Create Optimizely instance with specific configuration - Set up decision notification handler for optimalizely decision notifications - Test decision notification logic with holdout context - Validate decision notification data for various test scenarios
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. We should to keep the legacy API to avoid breaking changes if not necessary.
@@ -50,15 +50,15 @@ public final class ActivateNotification { | |||
* @param variation - The variation that was returned from activate. | |||
* @param event - The impression event that was triggered. | |||
*/ | |||
public ActivateNotification(Experiment experiment, String userId, Map<String, ?> attributes, Variation variation, LogEvent event) { | |||
public ActivateNotification(ExperimentCore experiment, String userId, Map<String, ?> attributes, Variation variation, LogEvent event) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we keep Experiment? I'm concerned about breaking changes in this public APIs.
This legacy API is not related to HOs, so no need to support HO type.
@@ -80,7 +82,7 @@ public final void handle(ActivateNotification message) { | |||
* @param variation - The variation that was returned from activate. | |||
* @param event - The impression event that was triggered. | |||
*/ | |||
public abstract void onActivate(@Nonnull Experiment experiment, | |||
public abstract void onActivate(@Nonnull ExperimentCore experiment, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
@@ -40,7 +41,7 @@ public interface ActivateNotificationListenerInterface { | |||
* @param variation - The variation that was returned from activate. | |||
* @param event - The impression event that was triggered. | |||
*/ | |||
public void onActivate(@Nonnull Experiment experiment, | |||
public void onActivate(@Nonnull ExperimentCore experiment, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same - this will be breaking changes
Summary
getVariationForHoldout
method added toDecisionService
class.holdout
as allowed decision sources.Test plan
Issues